Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block settings that restart the app when in form entry #6488

Merged
merged 6 commits into from
Nov 5, 2024

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Oct 30, 2024

Closes #6482

I've also cleaned up a couple of cases in FormFillingActivity that would return to the main menu in rare states.

Why is this the best possible solution? Were any other approaches considered?

As discussed in the issue and elsewhere, we decided to go with blocking these settings during form entry for now as reworking locking would be better to after #5420.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

The main things to check here are that the user is not able to access settings that reproduce the bug.

Before submitting this PR, please make sure you have:

  • added or modified tests for any new or changed behavior
  • run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
  • added a comment above any new strings describing it for translators
  • added any new strings with date formatting to DateFormatsTest
  • verified that any code or assets from external sources are properly credited in comments and/or in the about file.
  • verified that any new UI elements use theme colors. UI Components Style guidelines

// there is no formController -- fire MainMenu activity?
Timber.w("Starting MainMenuActivity because formController is null/formLoaderTask not null");
startActivity(new Intent(this, MainMenuActivity.class));
throw new IllegalStateException("Null formController!");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and the below case) don't have tests and as far as I'm aware, we don't know how they happen. Crashing means we don't have to deal with form session/lock clean up, and I actually think it's a better experience now that we show the crash message on the next relaunch than mysteriously returning to the Main Menu without warning.

@seadowg seadowg requested a review from grzesiek2010 October 31, 2024 10:11
@seadowg seadowg marked this pull request as ready for review October 31, 2024 10:11
@lognaturel lognaturel added the high priority Should be looked at before other PRs/issues label Oct 31, 2024

if (inFormEntry) {
findPreference<Preference>(PROJECT_MANAGEMENT_PREFERENCE_KEY)!!.also {
it.isEnabled = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be made clearer that these settings are disabled? The visual effect feels off - the title isn't grayed out, only the summary, which makes it hard to notice the difference at first glance.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree! We can tweak that if we need, so I'd vote we go with this for now and follow up if we're still unhappy with it.

@seadowg seadowg requested a review from grzesiek2010 November 5, 2024 11:08
@seadowg seadowg merged commit a1c16f9 into getodk:master Nov 5, 2024
6 checks passed
@seadowg seadowg deleted the exit-lock branch November 5, 2024 15:01
@dbemke
Copy link

dbemke commented Nov 6, 2024

Should "project management" (in settings) also be blocked during form entry?

@seadowg
Copy link
Member Author

seadowg commented Nov 6, 2024

Should "project management" (in settings) also be blocked during form entry?

Yes. Any settings that allow you to return to the main menu should. In project management you can reset the project, reconfigure with QR code etc which all return to the main menu.

@WKobus
Copy link

WKobus commented Nov 6, 2024

Tested with Success

Verified on device with Android 15

Verified cases:

@dbemke
Copy link

dbemke commented Nov 6, 2024

Tested with Success

Verified on device with Android 10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior verified high priority Should be looked at before other PRs/issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing language/theme during form entry blocks both match exactly and opening the form
5 participants